Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump minimum version to Go 1.17 #3849

Merged
merged 7 commits into from
May 6, 2023
Merged

Bump minimum version to Go 1.17 #3849

merged 7 commits into from
May 6, 2023

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Apr 26, 2023

Description:

This updates the version in go.mod to Go 1.17 as it enables sparse module indexes and also allows us to upgrade golang.org/x packages to later versions. Workflows have also been updated to test with Go 1.17 as a base.

The PR is intentionally small. Larger code changes can be done in the future.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Any breaking changes have a deprecation path or have been discussed.

@coveralls
Copy link

coveralls commented Apr 26, 2023

Coverage Status

Coverage: 61.982% (-0.006%) from 61.988% when pulling eb44f60 on Jacalz:go1.17 into 7de9188 on fyne-io:develop.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a bug warning about breaking old versions in the CHANGELOG file

@Jacalz
Copy link
Member Author

Jacalz commented Apr 27, 2023

@lucor Do you know why the license check workflow is suddenly failing?
I get the same error locally but I don't understand why it wasn't a problem before (or how we should fix it).

@Jacalz
Copy link
Member Author

Jacalz commented Apr 27, 2023

Sorry for the force push. I had the wrong settings on my laptop, resulting in the commit not being marked as "verified".

@andydotxyz
Copy link
Member

@lucor Do you know why the license check workflow is suddenly failing?
I get the same error locally but I don't understand why it wasn't a problem before (or how we should fix it).

It looks like the output points at the gobmp project not having a license file that is supported.
Perhaps that skirted under the radar before because the implicit dependencies were not checked directly?
You can see it appear in the go.mod with this change so I suspect that is what triggered it.

@lucor
Copy link
Member

lucor commented Apr 27, 2023

As @andydotxyz said missed LICENSE file. @Jacalz if your PR won't merged for any reason as workaround we'd exclude the check for that repo with the exclude flag.

@Jacalz
Copy link
Member Author

Jacalz commented Apr 27, 2023

Thanks @lucor. I have opened a PR upstream but I think it would be good to exclude the project from the check for now.

@Jacalz
Copy link
Member Author

Jacalz commented Apr 27, 2023

I managed to exclude the problematic repository but we are now running into issues with dependencies that are dual licensed with Unlicense (looking at you go-text) as well as a few projects using the ISC license. Should we allow those?
Allowing ISC seems fine to me but I don't know that do to about dual licensing projects.

@Jacalz
Copy link
Member Author

Jacalz commented Apr 27, 2023

Never mind. It is smart enough to understand that dual licensing is fine (it was still listed as not allowed locally so I thought it would kill the workflow). Nicely done @lucor :)

@Jacalz Jacalz requested a review from andydotxyz April 27, 2023 18:07
@andydotxyz
Copy link
Member

I guess these test hangs open the question - when do we update our branch rules to 1.17?
Clearly not yet as there is a release in progress... but do we do it immediately assuming there is either no more 2.3.x releases or that we don't accidentally break Go 1.14 support in the time between 2.3.4 and 2.4.0?

@Jacalz
Copy link
Member Author

Jacalz commented May 2, 2023

I think the best solution is to turn off the requirement on the Go 1.14 tests and just be extra careful to check them before merging anything into the release or master branches.

As long as we do not enable a requirement on the Go 1.17 checks, we can still have the Go 1.14 tests running on the other branches without further changes there.

We might be able to rename the workflows in the future and avoid running into the same issue next time we update the minimum.

@andydotxyz
Copy link
Member

Yeah if we could crack the naming that would rock!

@Jacalz
Copy link
Member Author

Jacalz commented May 6, 2023

I have now looked at improving the names but I don't see any way to get better naming without naming all the tests to the same name (which would be confusing) or to give them a better name format (but with the same problem occurring) using https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#run-name.

What I had in mind was to call the Go 1.17 tests something like Minimum required Go version tests but it does not seem to be possible without moving them into a separate workflow.

How should we proceed with this? I would still vote for turning off the requirement on Go 1.14 and not turn on the requirement for Go 1.17 tests once v2.4.0 go live in June.

FYI: I had to rebase to fix a conflict in the changelog and I also fixed the spelling in one of the commit messages, hence the force push.

@andydotxyz
Copy link
Member

Ah well, good to try. I have now turned off the requirement for 1.14 tests to pass before merge is allowed.
Let's remain vigilant until we can switch on 1.17 requirement after 2.4.0 release :).

@andydotxyz andydotxyz merged commit b1d74e2 into fyne-io:develop May 6, 2023
@Jacalz Jacalz deleted the go1.17 branch May 6, 2023 14:23
@Jacalz
Copy link
Member Author

Jacalz commented May 6, 2023

Indeed. Sounds good. I will keep an eye out for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants